[JavaWorker]Changes to the directory under src for support java worker by salah-man · Pull Request #2093 · ray-project/ray

salah-man · 2018-05-18T05:33:27Z

This commit includes changes to the directory under src, which is part of the java worker support of Ray.
It consists of the following changes:
src/common/task.cc - just fix null point problem
org_ray_spi_impl_DefaultLocalSchedulerClient.* - JNI support for local scheduler client, and the org_ray_spi_impl_DefaultLocalSchedulerClient.cc file is not autogenerated

-------------------------- This commit includes changes to the directory under src, which is part of the java worker support of Ray. It consists of the following changes: src/common/task.cc - just fix null point problem org_ray_spi_impl_DefaultLocalSchedulerClient.* - JNI support for local scheduler client, and the org_ray_spi_impl_DefaultLocalSchedulerClient.cc file is not autogenerated

AmplabJenkins · 2018-05-18T06:41:30Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/5472/
Test PASSed.

robertnishihara · 2018-05-18T07:07:26Z

#2095

AmplabJenkins · 2018-05-21T09:01:24Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/5536/
Test PASSed.

AmplabJenkins · 2018-05-21T15:32:08Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/5540/
Test PASSed.

robertnishihara · 2018-05-21T19:39:35Z

src/common/task.cc

+  if (ids == nullptr)
+    return 0;
+  else
+    return ids->size();


The linting doesn't seem to be enforcing this, but please change this to

if (ids == nullptr) { return 0; } else { return ids->size(); }

Got it, thanks for your time, i will change them soon

robertnishihara · 2018-05-21T19:39:41Z

src/local_scheduler/lib/java/org_ray_spi_impl_DefaultLocalSchedulerClient.cc

@@ -0,0 +1,224 @@
+


remove newline at beginning

robertnishihara · 2018-05-21T19:40:03Z

src/local_scheduler/lib/java/org_ray_spi_impl_DefaultLocalSchedulerClient.cc

+    _bytes = wid;
+
+    jbyte *b = (jbyte *) _env->GetByteArrayElements(_bytes, NULL);
+    PID = (UniqueID *) b;


In this file, let's remove all C style casts and instead use static_cast

AmplabJenkins · 2018-05-22T01:30:47Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/5551/
Test PASSed.

AmplabJenkins · 2018-05-22T02:56:35Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/5552/
Test FAILed.

AmplabJenkins · 2018-05-22T04:19:07Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/5554/
Test PASSed.

salah-man · 2018-05-22T13:05:20Z

Excuse me, but i just fix the problem as the comment before, could you review this pr again? Thank you very much. @robertnishihara

robertnishihara

Looks good to me! I left a few more comments and some questions. Thanks!

robertnishihara · 2018-05-23T05:46:15Z

src/local_scheduler/lib/java/org_ray_spi_impl_DefaultLocalSchedulerClient.cc

+  const char *nativeString = env->GetStringUTFChars(sockName, JNI_FALSE);
+  auto client = LocalSchedulerConnection_init(nativeString, *w.PID, isWorker);
+  env->ReleaseStringUTFChars(sockName, nativeString);
+  return (jlong) client;


use C++ cast

robertnishihara · 2018-05-23T05:49:00Z

src/local_scheduler/lib/java/org_ray_spi_impl_DefaultLocalSchedulerClient.cc

+ * Signature: (J)[B
+ */
+JNIEXPORT jbyteArray JNICALL
+Java_org_ray_spi_impl_DefaultLocalSchedulerClient__1getTaskTodo(JNIEnv *env,


I'm a bit confused about where these function names come from. E.g., what does the 1 mean and the Todo?

Oh, it is auto generated by javah, which make the function name looks stange.

robertnishihara · 2018-05-23T05:50:18Z

src/local_scheduler/lib/java/org_ray_spi_impl_DefaultLocalSchedulerClient.cc

+  TaskSpec *task =
+      reinterpret_cast<char *>(env->GetDirectBufferAddress(buff)) + pos;
+  std::vector<ObjectID> execution_dependencies;
+  if (cursorId != NULL) {


In general, we should avoid using NULL and instead use nullptr for pointers. However, I haven't worked much with JNI so should it be different in this case?

I just fix them, it is pointer here

robertnishihara · 2018-05-23T05:52:44Z

src/local_scheduler/lib/java/org_ray_spi_impl_DefaultLocalSchedulerClient.cc

+                                                         jlong numGpus) {
+  // 	native private static long _init(String localSchedulerSocket,
+  //     byte[] workerId, byte[] actorId, boolean isWorker, long numGpus);
+  UniqueIdFromJByteArray w(env, wid), a(env, actorId);


In general I think we should be using much more informative variable names (e.g., full words) and avoiding abbreviations

robertnishihara · 2018-05-23T05:55:07Z

src/local_scheduler/lib/java/org_ray_spi_impl_DefaultLocalSchedulerClient.cc

+  jbyteArray result;
+  result = env->NewByteArray(task_size);
+  if (result == NULL) {
+    return NULL; /* out of memory error thrown */


Is the plan to let the caller check for this and throw a Java exception?

yes, currently this situation will throw a Exception in java

AmplabJenkins · 2018-05-23T08:10:12Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/5594/
Test FAILed.

AmplabJenkins · 2018-05-23T08:57:51Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/5595/
Test FAILed.

AmplabJenkins · 2018-05-23T12:11:37Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/5596/
Test PASSed.

AmplabJenkins · 2018-05-24T00:51:28Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/5603/
Test PASSed.

* master: [autoscaler] GCP node provider (ray-project#2061) [xray] Evict tasks from the lineage cache (ray-project#2152) [ASV] Add ray.init and simple Ray benchmarks (ray-project#2166) Re-encrypt key for uploading to S3 from travis to use travis-ci.com. (ray-project#2169) [rllib] Fix A3C PyTorch implementation (ray-project#2036) [JavaWorker] Do not kill local-scheduler-forked workers in RunManager.cleanup (ray-project#2151) Update Travis CI badge from travis-ci.org to travis-ci.com. (ray-project#2155) Implement Python global state API for xray. (ray-project#2125) [xray] Improve flush algorithm for the lineage cache (ray-project#2130) Fix support for actor classmethods (ray-project#2146) Add empty df test (ray-project#1879) [JavaWorker] Enable java worker support (ray-project#2094) [DataFrame] Fixing the code formatting of the tests (ray-project#2123) Update resource documentation (remove outdated limitations). (ray-project#2022) bugfix: use array redis_primary_addr out of its scope (ray-project#2139) Fix infinite retry in Push function. (ray-project#2133) [JavaWorker] Changes to the directory under src for support java worker (ray-project#2093) Integrate credis with Ray & route task table entries into credis. (ray-project#1841)

fix lint problem in ci

9218378

salah-man force-pushed the java_worker_mr2 branch from 0a49eee to 9218378 Compare May 21, 2018 14:20

robertnishihara reviewed May 21, 2018

View reviewed changes

lint change for the feedback

268b31a

salah-man force-pushed the java_worker_mr2 branch from aeb7000 to 268b31a Compare May 22, 2018 02:25

salah-man closed this May 22, 2018

salah-man reopened this May 22, 2018

fix lint error in ci

e9b039a

robertnishihara reviewed May 23, 2018

View reviewed changes

change for the feedback

1e0d4fc

salah-man force-pushed the java_worker_mr2 branch from 17d3dd5 to 1e0d4fc Compare May 23, 2018 08:25

salah-man closed this May 23, 2018

salah-man reopened this May 23, 2018

salah-man closed this May 23, 2018

salah-man reopened this May 23, 2018

salah-man closed this May 23, 2018

salah-man reopened this May 23, 2018

fix lint error

19dab9e

salah-man force-pushed the java_worker_mr2 branch from fbe9e25 to 19dab9e Compare May 23, 2018 23:36

robertnishihara approved these changes May 25, 2018

View reviewed changes

robertnishihara merged commit 5c2b2c7 into ray-project:master May 25, 2018

Comments

Conversation

salah-man commented May 18, 2018

Uh oh!

AmplabJenkins commented May 18, 2018

Uh oh!

robertnishihara commented May 18, 2018

Uh oh!

AmplabJenkins commented May 21, 2018

Uh oh!

AmplabJenkins commented May 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AmplabJenkins commented May 22, 2018

Uh oh!

AmplabJenkins commented May 22, 2018

Uh oh!

AmplabJenkins commented May 22, 2018

Uh oh!

salah-man commented May 22, 2018

Uh oh!

robertnishihara left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AmplabJenkins commented May 23, 2018

Uh oh!

AmplabJenkins commented May 23, 2018

Uh oh!

AmplabJenkins commented May 23, 2018

Uh oh!

AmplabJenkins commented May 24, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants